-
Notifications
You must be signed in to change notification settings - Fork 292
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Arc] Initial SLP support #7061
Conversation
It's not finished yet, but I need to know whether I am on the right track or not. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This already looks pretty nice! Some comments inline below.
|
||
LogicalResult vectorize(); | ||
// Store Isomorphic ops together | ||
std::unordered_map<std::string, SmallVector<Operation *>> candidates; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You might be able to turn this into a DenseMap
, which is common in LLVM code bases.
a502143
to
c1e0851
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! This looks very cool 😎 🥳
let dependentDialects = ["arc::ArcDialect", "comb::CombDialect", | ||
"hw::HWDialect"]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The dependent dialects field lists the dialects of operations that your pass creates in some builder.create<...>
call. If I haven't overlooked anything, your pass just creates arc.*
ops, so you should be fine with just the Arc dialect here:
let dependentDialects = ["arc::ArcDialect", "comb::CombDialect", | |
"hw::HWDialect"]; | |
let dependentDialects = ["arc::ArcDialect"]; |
using llvm::SmallVector; | ||
using mlir::Operation; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You may be able to delete these. I think we have re-exports for these in the circt
namespace.
using llvm::SmallVector; | |
using mlir::Operation; |
using Key = std::tuple<unsigned, StringRef, SmallVector<Type>, | ||
SmallVector<Type>, ArrayRef<NamedAttribute>>; | ||
|
||
Key computeKey(Operation *op, unsigned rank) { | ||
// The key = concat(op_rank, op_name, op_operands_types, op_result_types, | ||
// op_attrs) | ||
return std::make_tuple( | ||
rank, op->getName().getStringRef(), | ||
SmallVector<Type>(op->operand_type_begin(), op->operand_type_end()), | ||
SmallVector<Type>(op->result_type_begin(), op->result_type_end()), | ||
op->getAttrs()); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can directly use the attribute dictionary instead of an array of named attributes. That makes the key cheaper to compare: the dictionary is just a pointer (interned in the MLIRContext), so it's just a pointer comparison:
using Key = std::tuple<unsigned, StringRef, SmallVector<Type>, | |
SmallVector<Type>, ArrayRef<NamedAttribute>>; | |
Key computeKey(Operation *op, unsigned rank) { | |
// The key = concat(op_rank, op_name, op_operands_types, op_result_types, | |
// op_attrs) | |
return std::make_tuple( | |
rank, op->getName().getStringRef(), | |
SmallVector<Type>(op->operand_type_begin(), op->operand_type_end()), | |
SmallVector<Type>(op->result_type_begin(), op->result_type_end()), | |
op->getAttrs()); | |
} | |
using Key = std::tuple<unsigned, StringRef, SmallVector<Type>, | |
SmallVector<Type>, DictionaryAttr>; | |
Key computeKey(Operation *op, unsigned rank) { | |
// The key = concat(op_rank, op_name, op_operands_types, op_result_types, | |
// op_attrs) | |
return std::make_tuple( | |
rank, op->getName().getStringRef(), | |
SmallVector<Type>(op->operand_type_begin(), op->operand_type_end()), | |
SmallVector<Type>(op->result_type_begin(), op->result_type_end()), | |
op->getAttrDictionary()); | |
} |
I am working on adding the statistics too. |
co-authors: @fabianschuiki